fix(ingest/metabase): Fix for query template expressions and invalid URNs for Text Cards #10381
Conversation
github-actions
bot
added
ingestion
| query_patched = re.sub(r"\[\[.+\]\]", r" ", raw_query) | ||
|
|
||
| # replace {{FILTER}} with 1 | ||
| query_patched = re.sub(r"\{\{.+\}\}", r"1", query_patched) |
There was a problem hiding this comment.
Small improvement idea (which I assume you already indicated in the issue comment): we could theoretically put .+ into a matcher group and only replace it if it's a know parameter by matching it against card_details["parameters"].
Not sure if it's worth the effort, though, as non-replaced parameters would only cause the parser to fail and ignore the lineage for this card and there's likely no harm in globally replacing everything.
There was a problem hiding this comment.
Yes, I was considering doing that and decided against it to avoid introducing additional complexity without any practical benefit so far I could see. I mean, other than being more "correct" and pedantic.
The correct place to fix that is upstream, all we can do downstream is to apply workarounds and sanitization. An upstream "fix" should be rather an feature to query the dashboard and card characteristics such as dependencies / database objects being used but that is not trivial and IMHO unlikely to be implemented any time soon.
pulsar256
changed the title
please take a look if the approach is ok. If it is fine, i will add another set of tests for the other issue and perhaps cover more variants of the templated queries. |
|
@hsheth2 ready for round 2. |
There was a problem hiding this comment.
For tests for #9767, it would be sufficient to just add some unit tests that call strip_template_expressions
For #10380, since it's a fairly small change, it would also be better to modify the existing test instead of creating an entirely new one. The duplication will be harder to manage down the line
I have no problem undoing / removing the metabase mocked responses. I have considered monkey patching the existing mocked metabase responses I found it rather problematic given that I had no access to the metabase state to (re)generate those. In the end we want to test the ingestion with response payloads as close as possible to what metabase generates instead of hand crafted or patched responses. Hence I refactored the test suite to allow for side effect free (between the individual tests) usage of independent reponse mocks and I was looking into automating the capture process as well. Do you want me to keep the refactored fixtures based approach for the response map in place or should we go back to the global state in the test suite? |
|
(...)
@hsheth2 ready for the next round |
|
|
||
|
|
||
| @freeze_time(FROZEN_TIME) | ||
| def test_9767_templated_query_is_stripped( |
There was a problem hiding this comment.
| def test_9767_templated_query_is_stripped( | |
| def test_strip_template_expressions( |
Co-authored-by: Harshal Sheth <[email protected]>
|
@hsheth2 thanks for the review. included both suggested changes, rebased. |
hsheth2
added
the
merge-pending-ci
|
@hsheth2 thanks, I think the workflows need another approval. Perhaps rebasing resets that? |
|
@pulsar256 yup I believe it does. No need to rebase/merge master - we'll merge it once CI is green. |
hsheth2
changed the title
…URNs for Text Cards (datahub-project#10381) Co-authored-by: Harshal Sheth <[email protected]>
Fixes #10380
Fixes #9767
Checklist